Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DPE-4904: Storage race condition + TLS update #311

Merged
merged 6 commits into from
Aug 27, 2024
Merged

DPE-4904: Storage race condition + TLS update #311

merged 6 commits into from
Aug 27, 2024

Conversation

Gu1nness
Copy link
Contributor

@Gu1nness Gu1nness commented Aug 21, 2024 •
edited
Loading

Issue

  • There's a race condition between storage ready and pebble ready event.
  • The TLS library needed to be update to ensure the db is initialised before enabling TLS

Solution

  • Check for the storages to see if they are available
  • Add tests about that
  • Update TLS library

MiaAltieri
MiaAltieri requested changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@MiaAltieri MiaAltieri left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this issue is a result of a race condition between TLS restart and normal start with pebble ready then I would rather fix this issue at the root of the cause which is that TLS tries to start the database first by running restart. Instead it should wait to run any hook by checking db_initialised

@Gu1nness
Copy link
Contributor Author

Gu1nness commented Aug 21, 2024 •
edited
Loading

If this issue is a result of a race condition between TLS restart and normal start with pebble ready then I would rather fix this issue at the root of the cause which is that TLS tries to start the database first by running restart. Instead it should wait to run any hook by checking db_initialised

Sure.
It's indeed a better solution, I'll do that and see if that works better!
I however have seen that in your case here it happened before (in the mongodb-pebble-ready).
I would say the best would be to use the storage events to set the right permissions right away but I'd like your advice on it?

Gu1nness and others added 4 commits August 23, 2024 10:49
@Gu1nness
Merge branch '6/edge' into DPE-4904-files-permissions-errors
bd6a9aa
@MiaAltieri @Gu1nness
CHORE: charmcraft fetch lib (#321)
677c72d 
* udpate libs

* make updates for changes in lib

* make necessary changes in tests and src
@Gu1nness
Revert "feat: Check data permissions before restarting mongod"
ba4e8d4 
This reverts commit 0cf2152.
@Gu1nness
feat: Defer ready event until the storage is ready
10c6173 
Also update the mongodb tls library
@Gu1nness Gu1nness changed the title feat: Check data permissions before restarting mongod DPE-4904: Storage race condition + TLS update Aug 23, 2024
@Gu1nness Gu1nness requested a review from MiaAltieri August 23, 2024 13:26
@Gu1nness Gu1nness marked this pull request as ready for review August 23, 2024 13:26
MiaAltieri
MiaAltieri requested changes Aug 23, 2024
View reviewed changes
src/charm.py Show resolved Hide resolved
tests/unit/test_charm.py Show resolved Hide resolved
Mehdi-Bendriss
Mehdi-Bendriss approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Neha! that's a good one

@Gu1nness Gu1nness requested a review from MiaAltieri August 27, 2024 08:06
@Gu1nness Gu1nness merged commit 0141b7a into 6/edge Aug 27, 2024
23 of 24 checks passed
@Gu1nness Gu1nness deleted the DPE-4904-files-permissions-errors branch August 27, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mehdi-Bendriss Mehdi-Bendriss Mehdi-Bendriss approved these changes

@MiaAltieri MiaAltieri MiaAltieri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gu1nness @MiaAltieri @Mehdi-Bendriss